fix: attempt to read without locking when locking fails; Some file sy… - #205
Conversation
|
@tpietzsch currently this handles the reading case in a way I'm happy with. However, I think the another this I did is remove the |
That isn't true. There will be no NPE in try-with-resources if the |
| this(path, true); | ||
| } | ||
|
|
||
| FileLazyRead(final Path path, final Boolean requireLock ) { |
There was a problem hiding this comment.
Why the boxing? This should be boolean instead of Boolean.
There was a problem hiding this comment.
No good reason, just didn't notice. I agree
Your example is too trivial. The NPE arrises because the block is still executed. in your example, you are only using modifying your example, and the output: public static void main(String[] args) {
try (MyClosable c = create()) {
System.out.println("doing something with " + c);
c.toString();
}
System.out.println("done");
} |
|
Oh, then I misunderstood.
I thought that you mean the try-with-resources will try to
But then there is no problem. and I explicitly decided to do it this way ("this way" meaning that N5NoSuchKeyException will not be thrown by createReadData.) I considered the alternative, and code like this:n5/src/main/java/org/janelia/saalfeldlab/n5/shard/DefaultDatasetAccess.java Lines 192 to 199 in 95d70d9 would become a lot more convoluted. (The code in the try block needs to run, even if there is no existingReadData.)
If we decide to let |
|
We (@cmhulbert , @tpietzsch ) discussed having |
…stem mount circumstances can lead to files being read/writeable, but not lockable. In these cases we should attempt to read, without locking, and immediately materialize as a best-effort attempt at ensuring valid data Signed-off-by: Caleb Hulbert <cmhulbert@gmail.com>
…ead/write operations Signed-off-by: Caleb Hulbert <cmhulbert@gmail.com>
…, so we handle it as `null` in PositionValueAccess Signed-off-by: Caleb Hulbert <cmhulbert@gmail.com>
…ReadData` and `write` Signed-off-by: Caleb Hulbert <cmhulbert@gmail.com>
… regex Signed-off-by: Caleb Hulbert <cmhulbert@gmail.com>
Signed-off-by: Caleb Hulbert <cmhulbert@gmail.com>
ff03d33 to
760e5b6
Compare
Signed-off-by: Caleb Hulbert <cmhulbert@gmail.com>
…Access. In practice, it was not consistently used. Most `Paths` and `Files` static methods internally get a file system from the FileSystemProvider Signed-off-by: Caleb Hulbert <cmhulbert@gmail.com>
| @Override | ||
| public VolatileReadData get(final long[] key) throws N5IOException { | ||
| return kva.createReadData(absolutePath(key)); | ||
| try { |
There was a problem hiding this comment.
KVA throws N5NoSuchFileException, so we catch/return null for PositionKVA
| public FileSystemKeyValueAccess(final FileSystem fileSystem) { | ||
|
|
||
| this.fileSystem = fileSystem; | ||
| private static final IoPolicy ioPolicy = getIoPolicy(); |
There was a problem hiding this comment.
relevant code for configuring what I'm calling IoPolicy. currently support strict atomic, no locking attempt unsafe and fallback which tries with atomic, but tries again with unsafe if it fails.
| */ | ||
| public class FileSystemKeyValueAccess implements KeyValueAccess { | ||
|
|
||
| protected final FileSystem fileSystem; |
There was a problem hiding this comment.
remove fileSystem field. It was used inconsistently. Most file calls used Files or Paths which get the file system internally
| } | ||
| } | ||
| } | ||
| } catch (NoSuchFileException ignore) { |
There was a problem hiding this comment.
this is new; previously we threw an exception on NoSuchFile. My opinion is we should just return without complaining, since the file does not exist, which is the intention of delete.
| * if a locked channel could not be created | ||
| * @deprecated migrate to {@link KeyValueAccess#createReadData(String)} | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
deprecated lockForReading, lockForWriting
Signed-off-by: Caleb Hulbert <cmhulbert@gmail.com>
Signed-off-by: Caleb Hulbert <cmhulbert@gmail.com>
| private static final IoPolicy ioPolicy = getIoPolicy(); | ||
|
|
||
| private static IoPolicy getIoPolicy() { | ||
| String property = System.getProperty("n5.ioPolicy"); |
There was a problem hiding this comment.
Longer term, I'd prefer that this property be configurable at a per-KVA instance (or maybe per KVA-class [1]) level rather than at a per-JVM level.
That being said, it won't bother me too much if this is how we do it for this release.
Signed-off-by: Caleb Hulbert <cmhulbert@gmail.com>
See #204 for more details.